Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Read-only mode for controllers #6259

Merged
merged 12 commits into from
Apr 20, 2023
Merged

Read-only mode for controllers #6259

merged 12 commits into from
Apr 20, 2023

Conversation

vitvakatu
Copy link
Contributor

@vitvakatu vitvakatu commented Apr 13, 2023 •
edited
Loading

Pull Request Description

Closes #6181

  • Added a read-only flag for the project model (now controlled by a temporary shortcut).
  • Renaming the project, editing the code, connecting and disconnecting nodes, and collapsing, and navigating through collapsed nodes are forbidden and will result in the error message in the console.
  • Some of the above actions produce undesired effects on the IDE, which will be fixed later. This PR is focused on restricting actual AST modifications.
  • Moving nodes (and updating metadata in general) no longer causes reevaluation
2023-04-11.18-07-19.mp4

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@vitvakatu vitvakatu self-assigned this Apr 13, 2023
@vitvakatu vitvakatu marked this pull request as ready for review April 13, 2023 00:47
farmaazon
farmaazon approved these changes Apr 13, 2023
View reviewed changes
Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test it before merge! The checkboxes are not checked.

app/gui/src/controller/graph/executed.rs Outdated Show resolved Hide resolved
@MichaelMauderer
Copy link
Contributor

QA report 🔴

When opening a project with this PR the state gets stuck at "Compiling Standard Library". This is not the case on develop.

image

@vitvakatu vitvakatu added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 13, 2023
@vitvakatu
Copy link
Contributor Author

@MichaelMauderer I can't reproduce it. I tried two different engines (3.30 and 4.13), and also tried both this branch and the current develop.

I triggered a similar-looking bug only once, on develop and engine 4.13. I can't reproduce it anymore, though. Could you check if it is persistent for you, and if it happens with the specific project or not?

@MichaelMauderer
Copy link
Contributor

anymore, though. Could you check if it is persistent for you, and if it happens with the specific project or not?

I just tried this on another machine with (a) a different project and (b) a new empty project. I'm running into the same issue there. Maybe we should get someone else to check this? Could it be platform-specific?

@vitvakatu
Copy link
Contributor Author

I found the issue, and it looks like the Engine behavior causes it. I wonder if I triggered the precisely same problem @MichaelMauderer has. I found only one reliable way to reproduce the issue: creating a new project. Opening an existing project caused the issue only once in 20+ tries, and I don't have logs for it (because I forgot to open dev tools in advance then).

What I see in network logs when I create a new project:

  1. We create execution context.
  2. We applyEdit with execute: true (it corresponds to adding an import statement for Standard.Visualization)
  3. A bit later, we applyEdit with execute: false (as we update the metadata only)
  4. The Engine never replies with executionComplete.

Screenshot 2023-04-18 at 13 52 47

When I open the existing project:

  1. There are no applyEdits from us.
  2. The Engine eventually replies with executionComplete, and everything is okay.

Screenshot 2023-04-18 at 13 56 10

So it looks like the Engine stops execution if we pass execute: false in one of the applyEdits. I suspect this is invalid, but I might be wrong. @4e6, could you take a look at this? I can reproduce it locally with this branch and engine version 4.18.

@vitvakatu
Copy link
Contributor Author

Merge is blocked by #6346

@wdanilo
Copy link
Member

wdanilo commented Apr 19, 2023

@vitvakatu if its blocked, please move it to draft PR until its unblocked. This way we easily see which PRs are ready for review/qa/merge :)

@vitvakatu vitvakatu marked this pull request as draft April 19, 2023 08:16
@vitvakatu vitvakatu marked this pull request as ready for review April 20, 2023 10:18
@vitvakatu
Copy link
Contributor Author

Merged with develop, I can't reproduce the issue anymore.

@Procrat
Copy link
Contributor

Procrat commented Apr 20, 2023

QA: Looks good to me (including creating a new project)!

@vitvakatu vitvakatu added the CI: Ready to merge This PR is eligible for automatic merge label Apr 20, 2023
@mergify mergify bot merged commit 6aba602 into develop Apr 20, 2023
@mergify mergify bot deleted the wip/vitvakatu/read-only-controllers branch April 20, 2023 14:17
Procrat added a commit that referenced this pull request Apr 25, 2023
@Procrat
Merge branch 'develop' into wip/procrat/rename-project-with-space-5065
04b228f 
* develop:
  Turn null into UnexpectedExpression when Union type is incomplete (#6415)
  Ensure that IO.println does not fail if `to_text` returned a non-Text value. (#6223)
  Improve inlining of `<|` on (GraalVM EE) (#6384)
  Feat: update templates styles and feature (#6071)
  5127 Add Table.parse_to_columns to parse a single column to a set of columns. (#6383)
  URL handling (#6243)
  Exclude comparison operators from modifier logic (#6370)
  Better cleanup of project management test suite (#6392)
  Fix all eslint errors (#6267)
  Infer SQLite types locally (#6381)
  Vector Editor with List Editor and adding elements. (#6363)
  Add typechecks to Aggregate and Cross Tab (#6380)
  Forbid edits in read-only mode (#6342)
  Add Table.parse_text_to_table to convert Text to a Table. (#6294)
  Adding typechecks to Column Operations (#6298)
  Rollback cloud options groups (#6331)
  Project folder not renamed after project name change (#6369)
  Add `replace`, `trim` to Column. Better number parsing. (#6253)
  Read-only mode for controllers (#6259)
  Prevent default for all events, fixing multiple IDE bugs (#6364)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@farmaazon farmaazon farmaazon approved these changes

@wdanilo wdanilo wdanilo approved these changes

@MichaelMauderer MichaelMauderer Awaiting requested review from MichaelMauderer

@kazcw kazcw Awaiting requested review from kazcw kazcw is a code owner

@mwu-tow mwu-tow Awaiting requested review from mwu-tow

Assignees

@vitvakatu vitvakatu

Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Forbid AST/execution context changes in controllers when read-only flag is set
5 participants
@vitvakatu @MichaelMauderer @wdanilo @Procrat @farmaazon